Refactor scheduler task concurrency evaluation into a dedicated helper.#67589
Refactor scheduler task concurrency evaluation into a dedicated helper.#67589SameerMesiah97 wants to merge 1 commit into
Conversation
boring-cyborg
Bot
added
the
area:Scheduler
potiuk
added
the
ready for maintainer review
|
Requesting review for this. |
jroachgolf84
left a comment
jroachgolf84
left a comment
There was a problem hiding this comment.
@SameerMesiah97 - I think we should add some tests to these changes.
Could you please elaborate on this? Do you want me to add tests for the helper itself? I could of course do that but my understanding is that the concurrency behaviour has already been covered by the existing tests. Keep in mind that this PR is introducing no behavioural changes. |
|
Requesting review for this. |
1 similar comment
|
Requesting review for this. |
steveahnahn
left a comment
steveahnahn
left a comment
There was a problem hiding this comment.
IMO this doesn't need new tests, the path is already pinned.
Nit: the "avoid loading the serialized DAG unless required" comment now lives inside the helper, where the load is unconditional (the has_task_concurrency_limits guard stayed at the call site). Move it back to the caller.
Move serialized DAG retrieval, concurrency checks, and starvation bookkeeping out of _executable_task_instances_to_queued.
aa8bc06 to
967d8a7
Compare
4 participants
Description
This change refactors task concurrency evaluation logic out of
SchedulerJobRunner._executable_task_instances_to_queuedinto a dedicated helper method.Task concurrency checks for
max_active_tis_per_dagandmax_active_tis_per_dagrun, along with their associated serialized DAG loading, starvation bookkeeping, and missing DAG handling, were previously implemented inline inside the main scheduling loop. This logic has now been extracted into_task_concurrency_allows_execution.The new helper encapsulates serialized DAG retrieval and missing DAG handling, task-level and task-per-DAG-run concurrency checks, and concurrency starvation bookkeeping and logging.
Rationale
_executable_task_instances_to_queuedcurrently performs a large number of responsibilities, including pool management, DAG-level concurrency enforcement, task-level concurrency evaluation, executor routing, starvation tracking, and queue state transitions.The task concurrency section in particular introduced deeply nested control flow and intermixed concurrency-specific behavior directly inside the main scheduling loop, making the scheduling path harder to visually scan and maintain.
Tests
No test adjustments were required because this change is a refactor only and does not introduce any behavioral changes.
Backwards Compatibility
This change is a refactor only and does not modify scheduler behavior, public APIs, task concurrency semantics, or executor interactions.
Was generative AI tooling used to co-author this PR?
Generated-by: [GPT 5.5] following the guidelines